Refactor high quality texture import #72031

reduz · 2023-01-25T11:23:45Z

Only two texture import modes for low/high quality now:
- S3TC/BPTC
- ETC2/ASTC
Makes sense given this is the general preferred and most compatible combination of texture compression formats in most platforms.
Removed lossy_quality from VRAM texture compression options. It was unused everywhere.
Added a new "high_quality" option to texture import. When enabled, it uses BPTC/ASTC (BC7/ASTC4x4) instead of S3TC/ETC2 (DXT1-5/ETC2,ETCA).
Changed MacOS export settings so required texture formats depend on the architecture selected.
Also did some fixes in ASTC support (mipmaps not supported).

This solves the following problems:

Makes it simpler to import textures as high quality, without having to worry about the specific format used.
On Apple, compressed texture format support is very difficult both for the editor and for exporting because, depending on the hardware, its either one or the other. This makes exporting and running the editor on MacOS "Just Work"(tm).
As the editor can now run on platforms such as web, Mac OS with Apple Silicion and Android, it should no longer be assumed that S3TC/BPTC is available by default for it.

editor/export/editor_export.cpp

editor/import/resource_importer_layered_texture.cpp

core/os/os.cpp

clayjohn · 2023-01-25T18:25:03Z

I'm getting a crash when loading a scene.

I deleted the .godot folder before trying to import. Then after crashing the first time I deleted the .import files as well and I received the same crash (albeit pointing to a different line number in ResourceImporterTexture::import

ERROR: Trying to unreference a SafeRefCount which is already zero is wrong and a symptom of it being misused.
Upon a SafeRefCount reaching zero any object whose lifetime is tied to it, as well as the ref count itself, must be destroyed.
Moreover, to guarantee that, no multiple threads should be racing to do the final unreferencing to zero.
   at: _check_unref_sanity (./core/templates/safe_refcount.h:176)


================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.0.beta.custom_build (dc5cdf4de4aa003de9e78780f87d7264aa4f9ec1)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7fd112c42520] (??:0)
[2] StringName::unref() (/home/clayjohn/dev/godot/./core/templates/safe_refcount.h:173)
[3] StringName::~StringName() (/home/clayjohn/dev/godot/./core/string/string_name.h:182)
[4] ResourceImporterTexture::import(String const&, String const&, HashMap<StringName, Variant, HashMapHasherDefault, HashMapComparatorDefault<StringName>, DefaultTypedAllocator<HashMapElement<StringName, Variant> > > const&, List<String, DefaultAllocator>*, List<String, DefaultAllocator>*, Variant*) (/home/clayjohn/dev/godot/editor/import/resource_importer_texture.cpp:454)
[5] EditorFileSystem::_reimport_file(String const&, HashMap<StringName, Variant, HashMapHasherDefault, HashMapComparatorDefault<StringName>, DefaultTypedAllocator<HashMapElement<StringName, Variant> > > const*, String const&) (/home/clayjohn/dev/godot/editor/editor_file_system.cpp:2001)
[6] EditorFileSystem::_reimport_thread(unsigned int, EditorFileSystem::ImportThreadData*) (/home/clayjohn/dev/godot/editor/editor_file_system.cpp:2159)
[7] WorkerThreadPool::GroupUserData<EditorFileSystem, void (EditorFileSystem::*)(unsigned int, EditorFileSystem::ImportThreadData*), EditorFileSystem::ImportThreadData*>::callback_indexed(unsigned int) (/home/clayjohn/dev/godot/./core/object/worker_thread_pool.h:152)
[8] WorkerThreadPool::_process_task(WorkerThreadPool::Task*) (/home/clayjohn/dev/godot/core/object/worker_thread_pool.cpp:72)
[9] WorkerThreadPool::_native_low_priority_thread_function(void*) (/home/clayjohn/dev/godot/core/object/worker_thread_pool.cpp:165)
[10] Thread::callback(Thread*, Thread::Settings const&, void (*)(void*), void*) (/home/clayjohn/dev/godot/core/os/thread.cpp:67)
[11] void std::__invoke_impl<void, void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>(std::__invoke_other, void (*&&)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*&&, Thread::Settings&&, void (*&&)(void*), void*&&) (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:61)
[12] std::__invoke_result<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>::type std::__invoke<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>(void (*&&)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*&&, Thread::Settings&&, void (*&&)(void*), void*&&) (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:96)
[13] void std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> >::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul>) (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:252)
[14] std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> >::operator()() (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:259)
[15] std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> > >::_M_run() (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:210)
[16] /home/clayjohn/dev/godot/bin/godot.linuxbsd.editor.dev.x86_64.llvm(+0xb2089f3) [0x5651a18e79f3] (msdf-error-correction.cpp:?)
[17] /lib/x86_64-linux-gnu/libc.so.6(+0x94b43) [0x7fd112c94b43] (??:0)
[18] /lib/x86_64-linux-gnu/libc.so.6(+0x126a00) [0x7fd112d26a00] (??:0)
-- END OF BACKTRACE --
============================

reduz · 2023-01-27T09:47:40Z

@clayjohn The crash and error seem unrelated to the PR, which is very strange. This PR forces the reimport of all textures, so it may be related to that?

clayjohn · 2023-01-27T18:20:58Z

@clayjohn The crash and error seem unrelated to the PR, which is very strange. This PR forces the reimport of all textures, so it may be related to that?

I'm not sure. I am testing on https://github.com/Rytelier/Godot-4-forest-benchmark and I can reliably reproduce the crash if I delete the .godot folder, or if import textures from an older version (I tried with Beta 15) and then try to import with this PR. Of note, after crashing, if I restart the editor it finishes importing the rest of the textures. So it seems like the issue could be due to the number of textures in the project?

I also tested with https://github.com/gdquest-demos/godot-4-3d-third-person-controller and it did not crash

Edit: Turns out in my testing project I had left "Import ETC" disabled. If I enable "Import ETC" I can reproduce the crash on master, so it is not caused by this PR

clayjohn · 2023-01-28T00:50:29Z

Couple notes:

When compressing Icon.png into ASTC I get the following error

astcenc: Encoding image size 64x64 to format ASTC_4x4, with mipmaps.
ERROR: Expected Image data size of 64x64x1 (with 6 mipmaps) = 5488 bytes, got 4096 bytes instead.
   at: initialize_data (core/io/image.cpp:2206)

It looks like the image size validator doesn't properly account for ASTC
2. I was able to work around the crash above by setting editor/import/use_multiple_threads to false. The crash happens more frequently when ETC ASTC is enabled. But it also happens on master with both S3TC and ETC2 enabled
3. The ASTC formats are missing from Image::format_names[]

godot/core/io/image.cpp

Lines 79 to 81 in a43db5a

    
           	"ETC2_RA_AS_RG", 
        
           	"FORMAT_DXT5_RA_AS_RG", 
        
           };

Need to add them to the end:

"ASTC_4x4",
"ASTC_4x4_HDR",
"ASTC_8x8",
"ASTC_8x8_HDR",

clayjohn · 2023-01-28T01:35:58Z

Here is the error log from running on an android device:

01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Unable to open file: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load_data (scene/resources/texture.cpp:846)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading resource: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex. Make sure resources have been imported by opening the project in the editor at least once.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load (core/io/resource_loader.cpp:222)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading resource: res://icon.svg. Make sure resources have been imported by opening the project in the editor at least once.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load (core/io/resource_loader.cpp:222)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Can't load dependency: res://icon.svg.
01-27 17:26:57.152 30718 30753 E godot   :    at: load (core/io/resource_format_binary.cpp:696)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading resource: res://.godot/exported/133200997/export-3070c538c03ee49b7677ff960a3f5195-main.scn. Make sure resources have been imported by opening the project in the editor at least once.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load (core/io/resource_loader.cpp:222)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading scene: res://main.tscn
01-27 17:26:57.152 30718 30753 E godot   :    at: start (main/main.cpp:2921)
01-27 17:26:57.153 30718 30753 E godot   : USER ERROR: Condition "default_certs != nullptr" is true.
01-27 17:26:57.153 30718 30753 E godot   :    at: load_default_certificates (modules/mbedtls/crypto_mbedtls.cpp:302)

Running on a Pixel 4. The scene is just the default icon on a Sprite2D taking up most of the screen. The icon is set to use high quality import. and only "ETC ASTC" import is enabled in the project settings (edit: same results if both "S3TC BPTC" and "ETC ASTC" enabled in project settings)
ASTC-test.zip

edit edit: Tested on Vulkan backend, same result. The project won't load and I get the above error endlessly. Unchecking the "high quality" flag on texture import makes the project work

reduz · 2023-01-28T17:27:57Z

@clayjohn Fixed some problems in the ASTC implementation, so hoping it works now. I can't manage to load ASTC on desktop (Godot does not recognize the format for some reason).

clayjohn · 2023-01-28T18:37:57Z

@clayjohn Fixed some problems in the ASTC implementation, so hoping it works now. I can't manage to load ASTC on desktop (Godot does not recognize the format for some reason).

Same error as before. The file it is referencing exists, its almost like its not getting included in the APK though. Do we have logic that selects what imported resources are included in the APK?

01-28 10:35:36.267 11948 11980 E godot   : USER ERROR: Unable to open file: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex.
01-28 10:35:36.267 11948 11980 E godot   :    at: _load_data (scene/resources/texture.cpp:846)
01-28 10:35:36.267 11948 11980 E godot   : USER ERROR: Failed loading resource: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex. Make sure resources have been imported by opening the project in the editor at least once.

* Only two texture import modes for low/high quality now: * S3TC/BPTC * ETC2/ASTC * Makes sense given this is the general preferred and most compatible combination in most platforms. * Removed lossy_quality from VRAM texture compression options. It was unused everywhere. * Added a new "high_quality" option to texture import. When enabled, it uses BPTC/ASTC (BC7/ASTC4x4) instead of S3TC/ETC2 (DXT1-5/ETC2,ETCA). * Changed MacOS export settings so required texture formats depend on the architecture selected. This solves the following problems: * Makes it simpler to import textures as high quality, without having to worry about the specific format used. * As the editor can now run on platforms such as web, Mac OS with Apple Silicion and Android, it should no longer be assumed that S3TC/BPTC is available by default for it.

reduz · 2023-01-30T14:54:29Z

@clayjohn Okay that took me a couple of hours to figure out, but it should be working well now. Make sure to test, but I tested on AMD (which supports ASTC) and it works fine.

akien-mga

Looks good to me. I would merge ASAP to get it properly tested in beta 17, as I expect bugs on different platforms :)

I'll wait to see if @clayjohn can at least confirm it works on his Android device.

reduz · 2023-01-30T17:25:37Z

@akien-mga I think its safe to merge at this point, seems to work well enough for me.

clayjohn · 2023-01-30T17:30:01Z

Just tested on my android device and I'm still getting the same output as before

clayjohn

Looks good to me. After re-importing and re-exporting with the newest changes this seems to work on Android both in mobile and gl_compatibility

jcostello · 2023-01-30T19:08:11Z

Dumb question but this has a visual impact?

clayjohn · 2023-01-30T19:33:45Z

Dumb question but this has a visual impact?

Yes, same as switching from s3tc to bptc in current betas. There won't be a visual change in the default case though

akien-mga · 2023-01-30T19:41:41Z

Thanks!

This is now required after godotengine#72031 when using HDRs. Could have further cleanup as I think these import options may not be needed at all anymore, and etc/etc2 support doesn't seem to make much sense. Likewise, the hardcoded "s3tc" in `get_platform_features` could maybe be removed. But this is material for after 4.1. Fixes godotengine#73789.

reduz requested review from a team as code owners January 25, 2023 11:23

akien-mga reviewed Jan 25, 2023

View reviewed changes

editor/export/editor_export.cpp Outdated Show resolved Hide resolved

akien-mga reviewed Jan 25, 2023

View reviewed changes

editor/export/editor_export.cpp Outdated Show resolved Hide resolved

akien-mga reviewed Jan 25, 2023

View reviewed changes

editor/import/resource_importer_layered_texture.cpp Show resolved Hide resolved

reduz force-pushed the change-high-quality-texture-import branch from 3069bce to 04b31d5 Compare January 25, 2023 11:41

bruvzg reviewed Jan 25, 2023

View reviewed changes

core/os/os.cpp Show resolved Hide resolved

reduz force-pushed the change-high-quality-texture-import branch from 04b31d5 to 6097a47 Compare January 25, 2023 12:02

akien-mga added enhancement topic:porting topic:import breaks compat labels Jan 25, 2023

akien-mga added this to the 4.0 milestone Jan 25, 2023

Calinou added the topic:3d label Jan 25, 2023

reduz force-pushed the change-high-quality-texture-import branch 2 times, most recently from 5bae6e5 to dc5cdf4 Compare January 25, 2023 15:13

reduz force-pushed the change-high-quality-texture-import branch from dc5cdf4 to 5ad04e1 Compare January 27, 2023 10:04

akien-mga added the needs testing label Jan 27, 2023

reduz force-pushed the change-high-quality-texture-import branch 2 times, most recently from eeb9353 to 12c6d5f Compare January 28, 2023 17:26

reduz force-pushed the change-high-quality-texture-import branch from 142969f to 28f51ba Compare January 30, 2023 14:53

reduz requested a review from a team as a code owner January 30, 2023 14:53

akien-mga approved these changes Jan 30, 2023

View reviewed changes

clayjohn approved these changes Jan 30, 2023

View reviewed changes

akien-mga merged commit e9de988 into godotengine:master Jan 30, 2023

akien-mga mentioned this pull request Feb 3, 2023

LightmapGI is slow to import due to OpenEXR VRAM compression being enabled by default #72666

Closed

f8122dac91 mentioned this pull request Feb 7, 2023

godot-upgrade: 4.0 beta 16 -> beta 17 igneous-labs/telegram-space#19

Closed

This was referenced Feb 7, 2023

Add ASTC to image and VRAM compression for better quality on mobile platforms godotengine/godot-proposals#5313

Closed

Allow importing textures as 16 bpc integer format godotengine/godot-proposals#5418

Closed

akien-mga mentioned this pull request Feb 11, 2023

Godot 4.0rc1 Editor gets stuck when importing files #73107

Closed

YuriSizov mentioned this pull request Feb 11, 2023

[4.0] Error importing resources by drag and drop #53871

Closed

RedworkDE mentioned this pull request Feb 14, 2023

4.0beta17/4.0rc1 Project stopped working in exported version after upgrade #73267

Closed

akien-mga mentioned this pull request Feb 23, 2023

Export: Default to exporting S3TC + BPTC for PC platforms #73829

Merged

Spartan322 mentioned this pull request Feb 27, 2023

Update Keychain SVG imports to Godot 4 RC 5 OpenVicProject/OpenVic#43

Merged

cridenour mentioned this pull request Mar 8, 2023

Constant project.godot and re-importing when using MacOS and Windows #74609

Closed

aaronfranke mentioned this pull request May 15, 2023

Enable S3TC_BPTC but not ETC2_ASTC by default #77105

Merged

BlueCube3310 mentioned this pull request Jul 21, 2024

When enabling BPTC format in a project, Godot doesn't pick the BC4 compressing format for normal maps automatically anymore. #38927

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor high quality texture import #72031

Refactor high quality texture import #72031

reduz commented Jan 25, 2023 •

edited

Loading

clayjohn commented Jan 25, 2023

reduz commented Jan 27, 2023 •

edited

Loading

clayjohn commented Jan 27, 2023 •

edited

Loading

clayjohn commented Jan 28, 2023

clayjohn commented Jan 28, 2023 •

edited

Loading

reduz commented Jan 28, 2023

clayjohn commented Jan 28, 2023

reduz commented Jan 30, 2023

akien-mga left a comment

reduz commented Jan 30, 2023

clayjohn commented Jan 30, 2023 •

edited

Loading

clayjohn left a comment

jcostello commented Jan 30, 2023 •

edited

Loading

clayjohn commented Jan 30, 2023

akien-mga commented Jan 30, 2023

Refactor high quality texture import #72031

Refactor high quality texture import #72031

Conversation

reduz commented Jan 25, 2023 • edited Loading

clayjohn commented Jan 25, 2023

reduz commented Jan 27, 2023 • edited Loading

clayjohn commented Jan 27, 2023 • edited Loading

clayjohn commented Jan 28, 2023

clayjohn commented Jan 28, 2023 • edited Loading

reduz commented Jan 28, 2023

clayjohn commented Jan 28, 2023

reduz commented Jan 30, 2023

akien-mga left a comment

Choose a reason for hiding this comment

reduz commented Jan 30, 2023

clayjohn commented Jan 30, 2023 • edited Loading

clayjohn left a comment

Choose a reason for hiding this comment

jcostello commented Jan 30, 2023 • edited Loading

clayjohn commented Jan 30, 2023

akien-mga commented Jan 30, 2023

reduz commented Jan 25, 2023 •

edited

Loading

reduz commented Jan 27, 2023 •

edited

Loading

clayjohn commented Jan 27, 2023 •

edited

Loading

clayjohn commented Jan 28, 2023 •

edited

Loading

clayjohn commented Jan 30, 2023 •

edited

Loading

jcostello commented Jan 30, 2023 •

edited

Loading